Vectorized hash grouping by a single text column #7586
Conversation
Use the UMASH hashes that have a guaranteed lower bound on collisions as the hash table keys.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7586 +/- ##
==========================================
+ Coverage 80.06% 81.83% +1.76%
==========================================
Files 190 243 +53
Lines 37181 44828 +7647
Branches 9450 11184 +1734
==========================================
+ Hits 29770 36686 +6916
- Misses 2997 3732 +735
+ Partials 4414 4410 -4 ☔ View full report in Codecov by Sentry. |
| typedef struct BytesView | ||
| { | ||
| const uint8 *data; | ||
| uint32 len; | ||
| } BytesView; |
There was a problem hiding this comment.
Instead of defining a new type, can we use StringInfo and initReadOnlyStringInfo? Just an idea.
There was a problem hiding this comment.
Well, it's not a complex type, and StringInfo has some unrelated things, so I'd keep it as is.
| BytesView *restrict output_key = (BytesView *) output_key_ptr; | ||
| HASH_TABLE_KEY_TYPE *restrict hash_table_key = (HASH_TABLE_KEY_TYPE *) hash_table_key_ptr; | ||
|
|
||
| if (unlikely(params.single_grouping_column.decompression_type == DT_Scalar)) |
There was a problem hiding this comment.
Question for my own understanding (not asking for any changes for this PR):
This is deep into vector aggregation, so I would expect that this function only gets passed an arrow array. But now we need to check for different non-array formats, including impossible cases (e.g, DT_Iterator). If we only passed in arrays, these checks would not be necessary.
The arrow array format already supports everything we need. Even scalar/segmentby values can be represented by arrow arrays (e.g., run-end encoded).
Now we need this extra code to check for different formats/cases everywhere we reach into the data. Some of them shouldn't even be possible here.
IMO, the API to retrieve a value should be something:
Datum d = arrow_array_get_value_at(array, rownum, &isnull, &valuelen);
This function can easily check the encoding of the array (dict, runend, etc.) to retrieve the value requested.
There was a problem hiding this comment.
Yeah, I have already regretted supporting the scalar values throughout aggregation. Technically it should perform better, because it avoids creating e.g. arrow array with the same constant value for every row, and sometimes we perform the computations in a different way for scalar values. But the implementation complexity might be a little too high. Maybe I should look into removing this at least for the key column, and always materializing them into arrow arrays. I'm going to consider this after we merge the multi-column aggregation.
The external interface might still turn out to be more complex than what you suggest, and closer to the current CompressedColumnValues, because sometimes we have to statically generate the function that works e.g. with dictionary encoding specifically, and that won't be possible if we determine this inside an opaque callback. We can't call an opaque callback (i.e. a non-inlinable dynamic function) for every row because it's going to produce significantly less performant code.
| const int total_bytes = output_key.len + VARHDRSZ; | ||
| text *restrict stored = (text *) MemoryContextAlloc(policy->hashing.key_body_mctx, total_bytes); | ||
| SET_VARSIZE(stored, total_bytes); | ||
| memcpy(VARDATA(stored), output_key.data, output_key.len); |
There was a problem hiding this comment.
Suggest making use of PostgreSQL builtin function:
| const int total_bytes = output_key.len + VARHDRSZ; | |
| text *restrict stored = (text *) MemoryContextAlloc(policy->hashing.key_body_mctx, total_bytes); | |
| SET_VARSIZE(stored, total_bytes); | |
| memcpy(VARDATA(stored), output_key.data, output_key.len); | |
| MemoryConext oldmcxt = MemoryContextSwitchTo(policy->hashing.key_body_mctx); | |
| text *stored = cstring_to_text_with_len(output_key.data, output_key.len); | |
| MemoryContextSwitchTo(oldmcxt); |
There was a problem hiding this comment.
I think it's better to have something that can be inlined here, because it's a part of a hot loop that builds the hash table.
| @@ -59,7 +59,7 @@ jobs: | |||
| build_type: ${{ fromJson(needs.config.outputs.build_type) }} | |||
| ignores: ["chunk_adaptive metadata telemetry"] | |||
| tsl_ignores: ["compression_algos"] | |||
| tsl_skips: ["bgw_db_scheduler bgw_db_scheduler_fixed"] | |||
| tsl_skips: ["vector_agg_text vector_agg_groupagg bgw_db_scheduler bgw_db_scheduler_fixed"] | |||
There was a problem hiding this comment.
Why do we need to skip these tests on windows? Is it also because of UMASH?
There was a problem hiding this comment.
Right, I didn't get it to compile there, so decided to disable for now.
Co-authored-by: Erik Nordström <[email protected]> Signed-off-by: Alexander Kuzmenkov <[email protected]>
Use the UMASH hashes that have a guaranteed lower bound on collisions as the hash table keys.
Up to 70% improvement in tsbench: https://grafana.ops.savannah-dev.timescale.com/d/fasYic_4z/compare-akuzm?orgId=1&var-branch=All&var-run1=4078&var-run2=4080&var-threshold=0.02&var-use_historical_thresholds=true&var-threshold_expression=2%20%2A%20percentile_cont%280.90%29&var-exact_suite_version=false&from=now-2d&to=now